fix(core): Report when migration patterns match no files - #5039
Conversation
`runMigrations()` returns an empty array both when every migration has already been applied and when the configured `migrations` glob patterns matched no files at all. The CLI derives its report from that array alone, so the second case is presented as "No pending migrations found" — a database that is out of sync with the entity schema looks identical to an up-to-date one. The patterns are resolved relative to the current working directory, so this happens when the command is run from an unexpected directory, or when the patterns point at compiled output that has not been built. Detect the case via the migration classes TypeORM actually loaded and hand the diagnostic to the caller through a new optional `RunMigrationsOptions.onNoMigrationsFound` callback. A callback is used rather than logging directly because `log()` is a no-op while running from the CLI, where a spinner is active for the duration of the call. Both CLI entry points surface the message in place of the misleading default. The exit code is deliberately unchanged: a freshly scaffolded project configures a `migrations` glob before any migration file exists, so failing here would break the default project template. Fixes vendurehq#5001
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe migration runner now accepts an optional callback for no-migration messages and provides a helper that reports unmatched configured patterns. The callback option is publicly exported. CLI migration commands capture the message and use it when no migrations are returned, while retaining the existing fallback for pending migrations being absent. New tests cover core message generation and CLI reporting outcomes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
biggamesmallworld
left a comment
There was a problem hiding this comment.
Thanks for this — the underlying observation is a good catch, and the PR description does a genuinely useful thing by writing down why the obvious fixes (log directly, throw) don't work. The onNoMigrationsFound seam is the right shape. Two things need to change before this lands, and I think one of them makes the PR considerably more valuable rather than smaller.
The signal is one step away from a better one
I built packages/core from this branch and ran the CLI e2e suite (e2e/vitest.e2e.config.mts), which the unit suite doesn't cover. One existing test fails:
FAIL runMigrationsOperation > should report no pending migrations when none exist
AssertionError: expected 'No migration files matched the config…' to contain 'No pending migrations found'
- No pending migrations found
+ No migration files matched the configured `migrations` patterns, so no migrations can be run.
+ Patterns are resolved relative to the current directory (…/e2e/fixtures/test-project):
+ - …/e2e/fixtures/test-project/migrations/*.ts
That isn't a stale assertion to update — it's the regression. The fixture has a valid glob over an empty directory, which is what every project looks like before its first migration is authored. connection.migrations.length === 0 overwhelmingly means "no migrations written yet", not "your globs are broken", so as written this prints an alarming, factually-true non-problem to exactly the users least able to evaluate it. You identified this scenario in the description (the create template configures the glob before any migration exists) and concluded it must not throw — but not throwing isn't the same as not being wrong.
There is a signal that discriminates exactly, and it's proof rather than heuristic: zero migration classes loaded while the migrations table has rows can only mean the globs stopped matching. That's also the dangerous real-world case — a deploy pointed at dist/migrations/*.js that was never built, against a database with history.
import { MigrationExecutor } from 'typeorm';
const executed = await new MigrationExecutor(connection).getExecutedMigrations();
if (!connection.migrations.length && executed.length) {
// globs definitively broken
}With that gate, the failing test above passes untouched, and the warning only fires when it's true.
I checked the fresh-database case since it's the one that would bite: against a sqlite DB with no migrations table, getExecutedMigrations() returns [] rather than throwing, so no special-casing is needed. It does create the migrations table as a side effect — which is fine at this call site, because connection.runMigrations() on the next line creates it anyway, but worth knowing before reusing the call somewhere read-only.
The bigger hole is already computed and thrown away
While you're in here: checkMigrationStatus() (migrate.ts:87) already computes builderLog.upQueries and emits precisely the warning #5001 asks for —
Your database schema does not match your current configuration. Generate a new migration for the following changes:
— via log(), which is a hard no-op whenever VENDURE_RUNNING_IN_CLI is set (migrate.ts:312). So on every single vendure migrate --run, the "your DB is out of sync" diagnostic is calculated and discarded. That is the data-safety gap in the issue, it costs nothing extra to detect, and the callback you've just added is the natural way to surface it. Routing that through the same seam would be the higher-value half of this change.
Worth noting the same suppression applies to revertLastMigration (migrate.ts:155), which the issue explicitly calls out as having identical symptoms.
Fixes #5001 isn't accurate
The reporter's glob matches — their workaround DataSource uses the identical pattern and applies migrations successfully — so connection.migrations.length is non-zero in their scenario and onNoMigrationsFound never fires. Their symptom is exit 0 in ~3s with no output at all, before any migration work happens.
I tried to reproduce that and could not. Every config-load failure I could construct against the e2e fixture exits non-zero with a message:
config = undefined → ■ Cannot read properties of undefined (reading 'plugins') EXIT=1
config with top-level await → ■ require() cannot be used on an ESM graph with top-level await EXIT=1
So that root cause is still open and needs a repro from the reporter (their tsconfig, plus NODE_DEBUG=module npx vendure migrate --run). Could you retarget this to Relates to #5001? Landing it as Fixes would close a live data-safety report that remains unaddressed.
Test that would have caught this
Nothing currently executes warnIfNoMigrationsFound — the CLI spec mocks @vendure/core wholesale, so runMigrations is a vi.fn(), and the core spec only exercises the message builder. The e2e fixture drives real TypeORM against real sqlite and is the right home. This passes on your branch and fails on master:
// #5001 — a `migrations` glob that resolves to nothing must not be reported as "up to date"
it('should report unmatched migration patterns when migrations have already been applied', async () => {
process.chdir(TEST_PROJECT_DIR);
// Apply a migration so the `migrations` table is non-empty
const generateResult = await generateMigrationOperation({
name: 'TestMigration',
outputDir: MIGRATIONS_DIR,
});
expect(generateResult.success).toBe(true);
expect((await runMigrationsOperation()).migrationsRan?.length).toBeGreaterThan(0);
// Simulate the glob resolving to nothing (wrong cwd, or unbuilt `dist/migrations/*.js`)
await fs.emptyDir(MIGRATIONS_DIR);
const result = await runMigrationsOperation();
expect(result.migrationsRan).toHaveLength(0);
expect(result.message).toContain('No migration files matched');
});Together with the existing 'should report no pending migrations when none exist', those two pin the discriminator from both sides.
One warning about verifying this locally: the CLI e2e suite resolves @vendure/core through packages/core/dist, so it will happily pass against a build that predates your change and tell you nothing. Run npm run build in packages/core first.
Smaller things
- Drop the two class-configuration tests in
migrate.spec.ts. If a migration class is inoptions.migrations, TypeORM loads it, soconnection.migrations.lengthcan't be0— those states are unreachable in production. They only typecheck becausegetNoMigrationsFoundMessagetakes the count and the config as independent parameters; passing the connection (or just the two derived booleans) would make them impossible to express. - The report-building block is now byte-identical in
migration-operations.ts:81-88andrun-migration/run-migration.ts:26-32, and'No pending migrations found'appears in three places plus an assertion inmigration-operations.spec.ts:194. Worth extracting into oneformatMigrationReport(). - Wrong channel.
run-migration.ts:33passes a multi-line message torunSpinner.stop()andmigrate.ts:87renders it withlog.success()— clack prefixes only the first line, so the pattern list hangs outside the box, and a warning gets a green success glyph with exit 0. If it's worth reporting, it isn't a success. - Public API.
RunMigrationsOptionsis exported frompackages/core/src/index.tswith@docsCategoryand@since, and its whole payload is a pre-formatted English string a programmatic consumer can only print. If it stays public, hand over data ({ patterns, cwd }, or the pending-change list) and let each caller format. Also preferexport type { RunMigrationsOptions }— the current value-export breaks downstream consumers building withisolatedModules. migration-operations-reporting.spec.ts:50(expect(...).not.toBe('No pending migrations found')) can't fail if line 49 passes. And that file duplicates the existingdescribe('runMigrationsOperation()')inmigration-operations.spec.tsunder a different mocking regime — worth merging.- The comment at
migrate.ts:141sayslog()is suppressed because a spinner is active. It's actually suppressed wheneverVENDURE_RUNNING_IN_CLIis set, spinner or not — themigrate --runpath (migrate.ts:62) has no spinner at all. Same for the ten-line block atmigrate.ts:104; most of it is commit-message material, and the one durable sentence is "TypeORM resolves migration globs relative toprocess.cwd()and silently yields zero classes when nothing matches."
Happy to review again once the gate changes — the diagnostic seam itself is good work and I'd like to see it land carrying the schema-drift warning too.
Description
runMigrations()returns an empty array in two different situations:migrationsglob patterns matched no files at all.Both CLI entry points derive their report from that array alone, so the second case is
presented as
No pending migrations found. A database that is out of sync with the entityschema is therefore indistinguishable from an up-to-date one, and
vendure migrate --runexits
0having applied nothing.The patterns are resolved relative to the current working directory, so this happens when the
command is run from an unexpected directory, or when the patterns point at compiled output
(e.g.
dist/migrations/*.js) that has not been built.Approach
The case is detectable from the migration classes TypeORM actually loaded (
connection.migrations),which is empty when the globs matched nothing. Where to report it needs some care:
log()inmigrate.tsis a no-op while running fromthe CLI, because a spinner is active for the duration of the call and writing to stdout
would corrupt it. That suppression is why the condition is currently invisible.
packages/create/templates/vendure-config.hbsconfiguresmigrations: [path.join(__dirname, './migrations/*.+(js|ts)')]before any migration fileexists, so a freshly created project would fail
vendure migrate --runwith a non-zero exit.So the diagnostic is handed to the caller through a new optional
RunMigrationsOptions.onNoMigrationsFoundcallback, and both CLI entry points surface it inplace of the misleading default message. This is additive and non-breaking; the exit code is
deliberately unchanged.
If you would prefer a non-zero exit here, that is a one-line change — happy to adjust.
Tests
packages/core/src/migrate.spec.ts(new file) — 7 tests covering the message builder:loaded migrations, unconfigured/empty patterns, class-valued entries, the object form of the
migrationsoption, and the reported patterns and cwd.packages/cli/src/commands/migrate/migration-operations-reporting.spec.ts(new file) —3 tests covering what the CLI actually reports. Verified that the first fails without this
change (
expected 'No pending migrations found' to be 'No migration files matched…') whilethe other two stay green.
Full
packages/clisuite passes (275 tests), andtsc --noEmitreports no new errors ineither package.
Fixes #5001
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.